Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of negative timestamps in Stat. #999

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Conversation

sunfishcode
Copy link
Member

Deprecate Stat::st_mtime etc., because they're unsigned and should be signed, and introduce a StatExt extension trait which adds mtime() etc. accessor functions which return the values in the appropriate signed type.

This StatExt trait can go away next time we have a semver breaking change, but for now this preserves compatibility.

Deprecate `Stat::st_mtime` etc., because they're unsigned and should be
signed, and introduce a `StatExt` extension trait which adds `mtime()`
etc. accessor functions which return the values in the appropriate
signed type.

This `StatExt` trait can go away next time we have a semver breaking
change, but for now this preserves compatibility.
@joshtriplett
Copy link
Member

Why a StatExt extension trait rather than implementing these methods directly on the structure?

@sunfishcode
Copy link
Member Author

Depending on the target and configuration, Stat is sometimes just a type alias for libc::stat or linux_raw_sys::general::stat, and sometimes a custom type. When it's a type alias, we can't define methods on it.

This is one of the things in progress for the next major version of rustix, to move to encapsulate all the libc/linux-raw-sys types, as having them in the public API causes a variety of problems. But that requires a breaking change too, so for now, an extension trait seems the best we can do.

@joshtriplett
Copy link
Member

@sunfishcode Ah, I didn't realize it was ever directly exposing the underlying type. Fair enough.

This looks like a sensible interim fix, then.

@sunfishcode sunfishcode merged commit 6ec74e0 into main Jan 19, 2024
44 checks passed
@sunfishcode sunfishcode deleted the sunfishcode/stat-ext branch January 19, 2024 00:18
@sunfishcode
Copy link
Member Author

This is now released in rustix 0.38.31.

@Byron
Copy link

Byron commented Feb 3, 2024

Thanks again for tackling this issue!

I was trying to use these improvements but had trouble doing so. The code looks like this (reproduced here for convenience)

            #[cfg(not(target_os = "aix"))]
            let seconds = self.0.st_mtime;
            #[cfg(target_os = "aix")]
            let seconds = self.0.st_mtim.tv_sec;

            // The fix I'd like to live without
            let seconds = seconds as i64;
            system_time_from_secs_nanos(seconds, nanoseconds.try_into().ok()?)

self.0 is of type rustix::fs::Stat. However, the trait implementation of rustix::fs::StatExt doesn't seem to provide an implementation for aix.

The solution seems to be to add such an implementation, but maybe there was a reason to not implement it in the first place.

In any way, without StatExt being implemented for all platforms I am probably better off leaving the gitoxide code as is, but with guidance I should be able to contribute what might be missing.

This also gave me the idea to use StatExt to taper over other code I had to write, but would rather not:

            #[cfg(not(any(target_os = "netbsd", target_os = "aix")))]
            let nanoseconds = self.0.st_mtime_nsec;
            #[cfg(target_os = "netbsd")]
            let nanoseconds = self.0.st_mtimensec;
            #[cfg(target_os = "aix")]
            let nanoseconds = self.0.st_mtim.tv_nsec;

Thanks for letting me know how to proceed here.

sthibaul added a commit to sthibaul/rustix that referenced this pull request May 26, 2024
6ec74e0 ("Fix handling of negative timestamps in `Stat`. (bytecodealliance#999)") broke
the hurd build, where we have a st_[acm]tim timespec instead of st_[acm],
like on aix and nto.
sthibaul added a commit to sthibaul/rustix that referenced this pull request May 26, 2024
6ec74e0 ("Fix handling of negative timestamps in `Stat`. (bytecodealliance#999)") broke
the hurd build, where we have a st_[acm]tim timespec instead of st_[acm],
like on aix and nto.
@sthibaul
Copy link
Contributor

In https://github.com/bytecodealliance/rustix/pull/1064/files I added the implementation

sunfishcode pushed a commit that referenced this pull request May 28, 2024
* hurd: Fix build

6ec74e0 ("Fix handling of negative timestamps in `Stat`. (#999)") broke
the hurd build, where we have a st_[acm]tim timespec instead of st_[acm],
like on aix and nto.

* aix, nto, hurd: Implement impl StatExt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants